where in icmpv4 rfc did you see such field for echo request?

That structure represents the echo req as specified from the RFC so you cannot modify it.

if you want to add data past the echo_req id/seq, you'll have to do it another way.

Yea you are right. I've changed it to a u8_t data[] and let the application decide what arbitrary payload to put in there (if any).

Not RFC compliant

use net_pkt_ieee802154_rssi() (btw, rssi for 15.4 is u8_t as it is a value between 0 and 255, why the cast?)

The data coming from the nrf52 is a s8_t tough, and RSSI is usually given in negative dBm.
The only other use is currently in openthread.c where it is implicitly cast to s8_t again.
I think this is a bug in Zephyr's ieee802154 API.

zephyr's 15.4 stack implements rssi as specified in 15.4 specs (8bits value from 0 to 255).
Ref: spec 2012 chapter 18.2.3.3

Though dbm would be indeed better (as it is the scale used also in BT and WiFi). I don't know why 15.4 came with its own definition.

Hm, with two's complement it doesn't make a difference either way, higher values are better no matter if they are interpreted as signed or not.

If the standard says so I'll remove the cast.

80 chars limit is passed here I guess

moved it to the next line

That's a bit problematic, because the target (if implementing reply wrongly) might have replied without the arbitrary data (it is against RFC but there is no way to verify this until you actually reach this cb).

Instead just use net_pkt_read_be32_new(), that will tell you if there is an error, in which case you will return NET_DROP.

Ok, changed it to use net_pkt_read_be32_new() instead.

So that won't be necessary then

removed

Even though, at this stage of this function implementation, this logic is not wrong, it won't fly with time if one day the icmpv4_access is not a contiguous one anymore.

And in any case, never memcpy (or memmove etc..) by yourself. Alway use the net_pkt r/w api, because that API can tell you if it could r/w or not (it verifies that buffer does have the necessary space)

So after net_pkt_set_data() below, just do a net_pkt_write(pkt, data, data_size)

Ok, changed it to net_pkt_write_new().

apply the same as for icmpv4.c

ok

80 chars limit

moved it to the next line

Hi @benpicco ,
About this payload, is it allocated from heap or thread stack or something else?
If from thread stack, is it possible to be overflowed with big size?

Using VLAs is no good, please do it differently. One option could be to allocate enough net_buf's for a given size, and then pass that chain of net_buf's directly to net_icmpv6_send_echo_request() which can then send that data directly.

Yes it's allocated on the stack and yes that's bad.

I'm not sure how to do it with net_bufs though. Is there a way where I don't have to reserve dedicated memory for it using NET_BUF_POOL_DEFINE but instead can make use of some existing pool?

Another way would be to only enable the -s option if CONFIG_HEAP_MEM_POOL_SIZE is not 0.

Hmm, we seem to have lost proper API in net_pkt.h for allocating net_buf's.

Another option could be to use net_pkt_skip() and optionally with net_pkt_memset(). That way we could fill enough net_buf's with given byte value. This would be needed to be done inside net_icmpv6_send_echo_request() as the net_pkt is not exposed outside of that function.

So for example we could have something like net_icmpv6_send_echo_request(iface, dst, id, seq, len, value), and inside that func just do net_pkt_skip(pkt, len) followed by cursor setting and then net_pkt_memset(pkt, value). The cursor setting is a bit vague here as I am just brainstorming atm.

After looking how net_pkt_skip() and net_pkt_memset() are implemented, it is probably enough just to call net_pkt_memset() which should allocate enough net_buf's to fill the packet.

Using VLAs is no good, please do it differently. One option could be to allocate enough net_buf's for a given size, and then pass that chain of net_buf's directly to net_icmpv6_send_echo_request() which can then send that data directly.

Print float data depends on some additional configs, otherwise, the time can not output correctly.

28 bytes from 192.168.1.1 to 192.168.1.100: icmp_seq=1 ttl=64 time=%.2f ms

I suppose it's CONFIG_NEWLIB_LIBC_FLOAT_PRINTF and it only affects the formatting to two significant digits as %f is used elsewhere in that file.

Does it work for you now?

It also doesn't work with %f, can you please use %d instead as this is a universal network tool.

28 bytes from 192.168.1.1 to 192.168.1.100: icmp_seq=0 ttl=64 time=%f ms

Ok.

What target / config are you using?
When I build for nrf52840_pca10056, CONFIG_NEWLIB_LIBC_FLOAT_PRINTF is not set, yet %.2f is available.

@benpicco I'm using 96boards IVY5661, the board support patch(#10626) is not merged yet.

I tried to store time in a variable before output, It's very strange that printf works, but PR_SHELL not. It seems there is some issue with PR_SHELL.

    float time;
    time =  SYS_CLOCK_HW_CYCLES_TO_NS(cycles) / 1000000.f;
    printf("time: %f.\n", time);

    PR_SHELL(shell_for_ping, "%d bytes from %s to %s: icmp_seq=%d ttl=%d time=%f ms\n",
         ntohs(ip_hdr->len) - net_pkt_ipv6_ext_len(pkt) - NET_ICMPH_LEN,
         net_sprint_ipv4_addr(&ip_hdr->src),
         net_sprint_ipv4_addr(&ip_hdr->dst),
         ntohs(icmp_echo->sequence),
         ip_hdr->ttl,
         time);

OUTPUT:
time: 4.418798.
28 bytes from 192.168.1.1 to 192.168.1.101: icmp_seq=0 ttl=64 time=%f ms

I tried this and noticed some issues. Using atoi() is not good as it returns 0 for invalid value. That is why the net-shell uses strtol() in other parts of the code.

Ok, converted it to use strtol()

Just noticed that we cannot stop the ping after it is started. So if the user puts very high value here, the shell will be useless for a long period of time. I wonder if we could run the ping in separate thread or something like that in order to avoid this thing. We can do some limits later, no need to start to implement something like that here.